(Improvements, need help testing) Rune related and span-ish optimizations #1247
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ations and optimized rune operations
Contributor
|
Personally most of the changes seem to make sense, but I would make the case that the Auth part does become way to complex with these changes (and also not sure how often that code even runs, cause I suppose it runs once per connection so probably not too hot of a path) The other parts do seem to make sense, especially the ReaderWriter optimizations, as those run for each query. Did you however happen to run the benchmarks against this to see what actual change it makes to performance? |
Author
|
Unfortunately, I haven't got to running benchmarks yet, however changes resulted in significant reduction of cpu time usage and allocations in application performance profiling runs, I will try to perform more thorough benchmarks and correctness tests soon, when I will have some free time |
Author
|
Also I agree that auth part is a case of over-optimization and can be omitted. I just applied change pattern to everything which allocates temporary buffers and I have got an eye on. So optimizations for things which run once per session/connection aren't necessary |
Author
|
Upd: I have run the Perf thing I found in a solution (idk if it is any representative) And yeah, the speed difference is pretty negligible, however reduction in allocations can be clearly observed Perf benchmarksAlso firebird 3 is used, disk used is OEM samsung nvme 2tb (pm9a1, aka oem 980 pro), 32gb of ddr4 ram @3200MT JEDEC, dual channel ofc |
…ed static, breaking tests)
Author
|
Upd2: Ran tests with firebird 3 (no embedded), so it does need further testing with other versions (especially embedded and batch operations in modern fb), there was an issue with boolean reading due to _smallbuffer being used both for reading useful bytes and pad (which doesn't affect types which don't get padded). Also, small test run time reduction was observed (aka 24.1 -> 23.5 mins, but without repeatability checks) and no changes in pass/failed/skipped numbers were noticed (after the fix) |
Author
|
Upd3: performed tests with embedded engine, all passed |
Author
|
Upd4: Practical benchmark resultsIt was a little bit tricky to actually obtain measurements which could show improvements, however some interesting observations can be made. |
…fixed size 16 byte arrays
Author
|
Upd5: Note: in the free time I have further tweaked XdrReaderWriter's buffer handling and simplified the code of AuthBlock Rune op benchmarksOfc. there are order of magnitude improvements when going from per rune allocations to allocation-free (aside from the instantiation of string from truncated span) methods, as the allocation and garbage collection of large amount of small objects is quite expensive. Benchmark of buffer handling in XdrReaderWriterThere are benchmarks comparing various methods of buffer handling, including: original, stackalloc by default (where available), using of preallocated smallBuffer (sizeof(data) <= 16) and using renting of ArrayPool. Also there are variants which perform erasing of data (clear variants) from buffers after operations for security. The main pattern in speed is: However in repeated operations only Synchronous operations are able to use stackalloc for IO buffer, while async operations have to use heap-allocated buffer, so the synchronous operations can benefit regardless of data size, while async operations can benefit from reduced allocations, but have slightly worse cpu performance for renting the small buffers, though in practice it shouldn't be measurable when dealing with actual IO, networking and db engine. Also there is a potential for implementing clean-up of data from buffers for hypothetical security improvements, as the performance impact of such action is not very big. Also the benchmarks were performed for two of the currently main .net versions (8 and 10) and in some cases .net 10's escape analysis improvements can avoid some of the heap allocations for array creation (see bool and int benchmarks), but the performance of stackalloc is still better, regardless of version, also .net 10 reduces performance impact from using ArrayPool. The only thing left to try is to change Array pool usage from global shared to static for XdrReaderWriter. Source for the benchmarks is available in perf project in the branch of my fork: local_opt2_benchmarks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I want to propose set of changes aimed at improving performance, which I have implemented and used for some time in my (private) projects.
The main goal of these changes is to significantly reduce allocations to heap by using stack allocations, array pool and avoiding unnecessary allocations in first place.
I have created topic in mailing list
I will appreciate opinions and help with testing, as I was used these changes for a while without any anomalies, though I didn't run thorough tests with all versions (I am using fb 3 server). Also the changes shouldn't have changed observable behavior.